GH-59313: Do not allow incomplete tuples, or tuples with NULLs.#117747
GH-59313: Do not allow incomplete tuples, or tuples with NULLs.#117747markshannon wants to merge 19 commits intopython:mainfrom
Conversation
Lib/test/test_tuple.py
Outdated
| for x in range(10): | ||
| yield x # SystemError when the tuple needs to be resized | ||
|
|
||
| with self.assertRaises(IndexError): |
There was a problem hiding this comment.
Would it be possible to rewrite the test to return somehow [x for x in gc.get_referrers(TAG) if isinstance(x, tuple)], and make then check that it's empty?
Example:
def my_iter():
yield TAG # 'tag' gets stored in the result tuple
yield [x for x in gc.get_referrers(TAG) if isinstance(x, tuple)]
for x in range(10):
yield x
result = tuple(my_iter())
self.assertEqual(result, (TAG, [], *range(10)))
Objects/tupleobject.c
Outdated
|
|
||
| PyObject * | ||
| _PyTuple_FromArraySteal(PyObject *const *src, Py_ssize_t n) | ||
| _PyTuple_FromNonEmptyArraySteal(PyObject *const *src, Py_ssize_t n) |
There was a problem hiding this comment.
I don't see how this change is related to the fix. And I don't see the benefits to reject n=0.
There was a problem hiding this comment.
I've reverted it.
_PyTuple_FromArraySteal is only called from the interpreter so we know that n != 0. I can change it in another PR.
vstinner
left a comment
vstinner
left a comment
There was a problem hiding this comment.
You should also update _PyTuple_Resize():
/* Zero out items added by growing */
if (newsize > oldsize)
memset(&sv->ob_item[oldsize], 0,
sizeof(*sv->ob_item) * (newsize - oldsize));| static inline void | ||
| PyTuple_SET_ITEM(PyObject *op, Py_ssize_t index, PyObject *value) { | ||
| PyTupleObject *tuple = _PyTuple_CAST(op); | ||
| assert(value != NULL); |
There was a problem hiding this comment.
You modified PyTuple_SET_ITEM() but not PyTuple_SetItem(), is it on purpose? I suggest to modify both, since PyTuple_SetItem() doesn't call PyTuple_SET_ITEM().
Doc/whatsnew/3.13.rst
Outdated
| (Contributed by Victor Stinner in :gh:`108765`.) | ||
|
|
||
| * The :c:func:`PyTuple_SET_ITEM` inline function may not be passed ``NULL``. | ||
| This has always been the documented behavior, but was not enforced for |
There was a problem hiding this comment.
https://docs.python.org/dev/c-api/tuple.html#c.PyTuple_SET_ITEM and https://docs.python.org/dev/c-api/tuple.html#c.PyTuple_SetItem don't say that the second argument must not be NULL. I suggest to modify the doc to be extra explicit about it.
Objects/abstract.c
Outdated
| @@ -2040,8 +2040,6 @@ PySequence_Tuple(PyObject *v) | |||
| { | |||
| PyObject *it; /* iter(v) */ | |||
| Py_ssize_t n; /* guess for result tuple size */ | |||
There was a problem hiding this comment.
There is a warning: unused variable ‘n’ [-Wunused-variable]
|
Overall, the change looks good to me, but I made a second review with more suggestions. |
|
|
||
| Like :c:func:`PyTuple_SetItem`, but does no error checking, and should *only* be | ||
| used to fill in brand new tuples. | ||
| used to fill in brand new tuples. Both ``p`` and ``o`` must be non-``NULL``. |
There was a problem hiding this comment.
Can you move this addition to PyTuple_SetItem(), since PyTuple_SET_ITEM() is "like PyTuple_SetItem()"?
Or just copy/paste the sentence in PyTuple_SetItem() doc?
|
This change seems to break weakrefs. Closing until I have time to investigate and fix it. |
|
It is possible that changing from |
3 participants
Prevents
gc.get_referrersfrom returning invalid tuples, and should make the cycle GC a bit more robust.Not using
NULLallows some efficiency improvements, like changingXDECREFtoDECREF.I've also streamlined
tuple_alloc, since I was fiddling with tuple creation and destruction anyway.PySequence_Tupletakes 0.02% of the runtime of the benchmark suite, so any change to its performance will be undetectable.